Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testDependency dir under C:/Users/jenkins/ should not be removed on windows #19791

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AswathySK
Copy link
Contributor

3rd party libs under C:/Users/jenkins/testDependency on Windows are pre-staged . However, this testDependency dir gets removed by this cleanup script since it removes patterns starting with test* in C:/Users/jenkins/ directory for windows machines.Due to missing pre-staged jars, the test jobs have to download the 3rd party jar at runtimes. The often cause test jobs failure because of unstable networks.

Helpful links:
https://github.ibm.com/runtimes/infrastructure/issues/9395

@AswathySK
Copy link
Contributor Author

@keithc-ca , @pshipton can you please take a look at this PR?

@keithc-ca keithc-ca self-requested a review July 3, 2024 13:32
@keithc-ca
Copy link
Contributor

This change is not only for Windows, the title and description should not suggest otherwise.

@AswathySK
Copy link
Contributor Author

This change is not only for Windows, the title and description should not suggest otherwise.

@keithc-ca , This issue is only for windows machines because in the script there is an additional portion for windows systems where they cleanup the /cygdrive/c/Users/jenkins/ directory where the testDependency is usually saved:

  cleanDirsStr += " ${buildWorkspace}/../../"
  cleanDirsStr += cleanDirs.join(" ${buildWorkspace}/../../")

Inorder to prevent this we should stop test* directories getting removed only from /cygdrive/c/Users/jenkins/ directory in windows.

This step could be a fix instead of the one I have made in this PR:

 if (nodeLabels.contains('sw.os.windows')) {
      // test resources
      cleanDirsStr += " ${buildWorkspace}/../../"
      cleanDirsStr += cleanDirs.join(" ${buildWorkspace}/../../")
      // shared classes cache
      cleanDirsStr += " ${buildWorkspace}/../../javasharedresources /tmp/javasharedresources /temp/javasharedresources"                                     
      cleanDirsStr = cleanDirsStr.replaceAll("${buildWorkspace}/../../test\\*", "")
                                    }

This way other test* directories will be removed from tmp for other OS and for windows testDependency directory wont be removed in /cygdrive/c/Users/jenkins/ directory.

@keithc-ca
Copy link
Contributor

Could we not arrange that testDependency is someplace (outside the workspace) so it won't be deleted?

@AswathySK
Copy link
Contributor Author

Could we not arrange that testDependency is someplace (outside the workspace) so it won't be deleted?

@llxia what are your thoughts on this?

@llxia
Copy link
Contributor

llxia commented Sep 4, 2024

testDependency dir is outside the Jenkins workspace. This clean-up job deletes files outside the workspace.

@AswathySK
Copy link
Contributor Author

@llxia , Only in windows the cleanup job is clearing directories under /cygdrive/c/Users/jenkins/. Is this step required in windows?
Does the tests create directories apart from testDependencies in /cygdrive/c/Users/jenkins/ directory which should be removed?

@keithc-ca
Copy link
Contributor

Ok, testDependency is outside the Jenkins workspace: shouldn't the cleanup job confine itself to the workspace? Then there would be no concern about removing files in testDependency that we want to keep.

@llxia
Copy link
Contributor

llxia commented Sep 4, 2024

Only in windows the cleanup job is clearing directories under /cygdrive/c/Users/jenkins/. Is this step required in windows?

I will leave @AdamBrousseau to comment on this one.

Does the tests create directories apart from testDependencies in /cygdrive/c/Users/jenkins/ directory which should be removed?

We have a separate clean-up job to periodically delete the test pipeline pre-staged files. This Cleanup-Nodes.groovy does not need to worry about any pre-staged files from the test pipeline under /cygdrive/c/Users/Jenkins/

shouldn't the cleanup job confine itself to the workspace?

I will try to answer this one. The purpose of Cleanup-Nodes.groovy is to remove any leftover files outside the workspace (i.e., from individual users, test jobs, etc). It tries to clean /tmp, /temp, /cores, etc.

@AdamBrousseau
Copy link
Contributor

Only in windows the cleanup job is clearing directories under /cygdrive/c/Users/jenkins/. Is this step required in windows?

We can turn it off and keep an eye on it. At some point we noticed there was stuff in $HOME on Windows so we added it as a cleanup location.

Many tests create artifacts in $TEMP=. The cleanWs() command does not handle these artifacts because it is outside of the $WORKSPACE. Other than TEMP and HOME (on Windows), we also remove everything in workspace dir in case there are jobs that failed (or forgot) to cleanup after themselves.

To resolve this PR can we not just change the code to explicitly exclude removing the testDependency dir?

@keithc-ca
Copy link
Contributor

change the code to explicitly exclude removing the testDependency dir

That was my request.

@AswathySK AswathySK force-pushed the testDependency branch 2 times, most recently from 6f421cc to 2d43983 Compare September 25, 2024 08:01
@AswathySK
Copy link
Contributor Author

@keithc-ca , The change has been made.
@AdamBrousseau, who can I approach to make a list of dirs/files which will get created in jenkins directories after running tests. I want to understand what all should be maintained and what should be removed from the home dir. For example, is jck_root something which we should not delete?

@AdamBrousseau
Copy link
Contributor

@AdamBrousseau, who can I approach to make a list of dirs/files which will get created in jenkins directories after running tests. I want to understand what all should be maintained and what should be removed from the home dir. For example, is jck_root something which we should not delete?

There should be nothing created by test in jenkins user's HOME dir. There is plenty created in /tmp as we know but we do not know enough about all the tests to know what test creates what artifact(s). @pshipton @llxia can confirm.
The main directories in HOME should be bootjdks, jck_root, openjdk_cache, workspace, and openj9_resources (only on z/os). I don't think we can take a blanket approach of deleting everything except certain directories though. That is too risky. We need to maintain a list of specific items we know are generated and not always cleaned up automatically and only remove those.

@@ -163,6 +163,9 @@ timeout(time: TIMEOUT_TIME.toInteger(), unit: TIMEOUT_UNITS) {
cleanDirsStr += cleanDirs.join(" ${buildWorkspace}/../../")
// shared classes cache
cleanDirsStr += " ${buildWorkspace}/../../javasharedresources /tmp/javasharedresources /temp/javasharedresources"
// to remove {buildWorkspace}/../../test* directories except testDependency
cleanDirsStr = cleanDirsStr.replaceAll("${buildWorkspace}/../../test\\*", "")
sh(script: "ls -d ${buildWorkspace}/../../test* | grep -v 'testDependency' | xargs rm -rf", returnStdout: true).trim()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The grep part of the pipeline should be grep -v '/testDependency$'.

@@ -163,6 +163,9 @@ timeout(time: TIMEOUT_TIME.toInteger(), unit: TIMEOUT_UNITS) {
cleanDirsStr += cleanDirs.join(" ${buildWorkspace}/../../")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This, or cleanDirs has to change; as is, this still includes ${buildWorkspace}/../../test* which will remove the testDependency directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keithc-ca cleanDirsStr = cleanDirsStr.replaceAll("${buildWorkspace}/../../test\\*", "") this line would exclude removing test* items from ${buildWorkspace}/../../ directory.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I missed that line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest a better approach would be to add 'test*' to cleanDirs only for non-Windows nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keithc-ca ,
I think even in windows we are required to clean test* dirs in /tmp directory. The only exclusion is for the testDependency dir. Is that not right @llxia @AdamBrousseau?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, are we agreed that we should change those jobs to use a different name than testDependency, say to external or support?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@llxia what should be the action taken?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @llxia. It will be good to raise an aqa-tests issue for awareness for others that the folder name will be changed to better support automatic cleaning of test material, while maintaining the benefit of preloading test dependencies. Other than communicating the change and checking that any references to that folder are accounted for, I have no objections.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants